Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[feature] Spy passes through calling with new #1626

Merged
merged 7 commits into from
Dec 7, 2017
Merged

[feature] Spy passes through calling with new #1626

merged 7 commits into from
Dec 7, 2017

Conversation

ProLoser
Copy link
Contributor

@ProLoser ProLoser commented Dec 6, 2017 •
edited
Loading

Purpose (TL;DR)

This changes the spy behavior so that if the spy is calledWithNew() then
the original function will also get called with new.

This was throwing an error if you tried spying on an ES6 class object.

Fixes #1265

How to verify

let x = {
  y: class {}
};

spy(x, 'y');

// This would normally throw during callThrough()
new x.y();

assert(x.y.calledWithNew());

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@ProLoser
[feature] Spy passes through calling with new
6b8b8b3 
This changes the spy behavior so that if the spy is `calledWithNew()` then
the original function will also get called with `new`.

This was throwing an error if you tried spying on an ES6 class object.
fatso83
fatso83 requested changes Dec 6, 2017
View reviewed changes
Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this seems like a good thing. It doesn't break any existing tests (do fix the linting though!), and fixes a valid case for ES2015.

It needs a verification test that shows this is working though!

lib/sinon/spy.js Outdated
} else {
returnValue = (this.func || func).apply(thisValue, args);
}

if (thisCall.calledWithNew() && typeof returnValue !== "object") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps nest this within the first check?

@ProLoser
Copy link
Contributor Author

ProLoser commented Dec 6, 2017

@fatso83 Updated.

Sorry for doing everything on the Github website vs a streamlined PR, didn't want to take time to clone/install/test/etc when it's all built into CI/CD

ProLoser and others added 3 commits December 6, 2017 10:53
fatso83
fatso83 approved these changes Dec 7, 2017
View reviewed changes
lib/sinon/spy.js Outdated
if (thisCall.calledWithNew()) {
// Call through with `new`
// eslint-disable-next-line new-parens
returnValue = new (Function.prototype.bind.apply(this.func || func, [thisValue].concat(args)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a list of methods from the various built-in above, so I'll extract the bind as a variable and move it up to be consistent and make it a bit more readable.

@fatso83 fatso83 merged commit 911c498 into sinonjs:master Dec 7, 2017
@fatso83
Copy link
Contributor

fatso83 commented Dec 7, 2017

Thank you! I had a hard time understanding what was going on, so needed to play with the code and run it in a debugger and examine it before I could accept it. Made a tiny change, but your test was sufficient to prove everything still worked.

Since the everything done here logically belonged in one commit it was ok that the history was a bit messy, as I could squash everything, including my commit. And yes, the unit test you added was fine :-)

@fatso83
Copy link
Contributor

fatso83 commented Dec 7, 2017

Released as 4.1.3

@ProLoser ProLoser deleted the patch-1 branch December 7, 2017 17:27
ProLoser
ProLoser commented Dec 7, 2017
View reviewed changes
lib/sinon/spy.js Outdated
@@ -195,7 +196,7 @@ var spyApi = {
if (thisCall.calledWithNew()) {
// Call through with `new`
// eslint-disable-next-line new-parens
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line could probably have been removed with your addition.

Copy link
Contributor

@fatso83 fatso83 Dec 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, true. thanks! Fixed in 6ba959e

@mroderick
Copy link
Member

Thank you!

fatso83 added a commit that referenced this pull request Dec 8, 2017
@fatso83
Remove unneeded eslint-disable line
6ba959e 
Was fixed by some minor changes in the squashed commit.
Ref #1626 (comment)
@fatso83 fatso83 mentioned this pull request Dec 8, 2017
Closed
@danxuliu danxuliu mentioned this pull request Dec 8, 2017
Merged
@PVince81 PVince81 mentioned this pull request Feb 11, 2018
Open
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
@ProLoser
Spy passes through calling with new (sinonjs#1626)
b4a5985 
This changes the spy behavior so that if the spy is `calledWithNew()` then
the original function will also get called with `new`.

This was throwing an error if you tried spying on an ES6 class object.

fixes sinonjs#1265
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
@fatso83
Remove unneeded eslint-disable line
1dc7f3d 
Was fixed by some minor changes in the squashed commit.
Ref sinonjs#1626 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fatso83 fatso83 fatso83 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ProLoser @fatso83 @mroderick